Add New Environment Variables for Load Balancer Configuration #1052
Conversation
|
Hey @M4t7e, just to confirm, the "8 Annotations" limit is from the I found this thread where the limit was discussed and added, it sounds like this is a "soft" limit that can be raised if a reasonable case is made for more than 8 annotations: kubernetes-sigs/gateway-api#1757 (comment) |
|
Hey @apricote, yes, that's the limit I was referring to. I began researching ways to preset annotations in GatewayAPI, since the concept is not to have a single Gateway (Load Balancer) for everything, like it is often the case for Ingress Controller, but to have the flexibility of creating multiple Gateways. To avoid repeating the same config, I was looking into options setting global annotation setting. That's when I came across this issue: kubernetes-sigs/gateway-api#2734 From what I understand, the annotation limit can only be increased through provider-specific implementations like Istio and Envoy already support. In my case, I’m planning to use Cilium GatewayAPI, which as far as I know doesn’t support adding annotations with their custom config. Btw, this is the actual issue where we want to add GatewayAPI support: hcloud-k8s/terraform-hcloud-kubernetes#216 |
|
I personally dislike the many annotations, and have often wondered how we could provide a better alternative. The Gateway API @lukasmetzner will be back next week to take a closer look at the MR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1052 +/- ##
==========================================
- Coverage 68.29% 64.76% -3.54%
==========================================
Files 23 23
Lines 2520 2608 +88
==========================================
- Hits 1721 1689 -32
- Misses 629 745 +116
- Partials 170 174 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
e2e test not passing is fine here. This is an expected permission issue. |
| } | ||
|
|
||
| if algorithmType, ok := os.LookupEnv(hcloudLoadBalancersAlgorithmType); ok { | ||
| alg, parseErr := parseLoadBalancerAlgorithmType(algorithmType) |
There was a problem hiding this comment.
I think we are mixing Read and Validate in this call. Could you please move the arg type validation into Validate.
| if subnetRange, ok := os.LookupEnv(hcloudLoadBalancersPrivateSubnetIPRange); ok { | ||
| cfg.LoadBalancer.PrivateSubnetIPRange = subnetRange | ||
| } |
There was a problem hiding this comment.
We could use net.ParseCIDR in the Validate function to check if the format is correct.
| if interval, ok := os.LookupEnv(hcloudLoadBalancersHealthCheckInterval); ok { | ||
| d, parseErr := time.ParseDuration(interval) | ||
| if parseErr != nil { | ||
| errs = append(errs, fmt.Errorf("failed to parse %s: %w", hcloudLoadBalancersHealthCheckInterval, parseErr)) | ||
| } else { | ||
| cfg.LoadBalancer.HealthCheckInterval = d | ||
| } | ||
| } |
There was a problem hiding this comment.
| if interval, ok := os.LookupEnv(hcloudLoadBalancersHealthCheckInterval); ok { | |
| d, parseErr := time.ParseDuration(interval) | |
| if parseErr != nil { | |
| errs = append(errs, fmt.Errorf("failed to parse %s: %w", hcloudLoadBalancersHealthCheckInterval, parseErr)) | |
| } else { | |
| cfg.LoadBalancer.HealthCheckInterval = d | |
| } | |
| } | |
| cfg.LoadBalancer.HealthCheckInterval, err = getEnvDuration(hcloudLoadBalancersHealthCheckInterval) | |
| if err != nil { | |
| errs = append(errs, err) | |
| } |
We already have a util function for this. If errs is not empty, HCCM won't continue and throw an error. So the if-else is not necessary here.
| if timeout, ok := os.LookupEnv(hcloudLoadBalancersHealthCheckTimeout); ok { | ||
| d, parseErr := time.ParseDuration(timeout) | ||
| if parseErr != nil { | ||
| errs = append(errs, fmt.Errorf("failed to parse %s: %w", hcloudLoadBalancersHealthCheckTimeout, parseErr)) | ||
| } else { | ||
| cfg.LoadBalancer.HealthCheckTimeout = d | ||
| } | ||
| } |
There was a problem hiding this comment.
| if timeout, ok := os.LookupEnv(hcloudLoadBalancersHealthCheckTimeout); ok { | |
| d, parseErr := time.ParseDuration(timeout) | |
| if parseErr != nil { | |
| errs = append(errs, fmt.Errorf("failed to parse %s: %w", hcloudLoadBalancersHealthCheckTimeout, parseErr)) | |
| } else { | |
| cfg.LoadBalancer.HealthCheckTimeout = d | |
| } | |
| } | |
| cfg.LoadBalancer.HealthCheckTimeout, err = getEnvDuration(hcloudLoadBalancersHealthCheckTimeout) | |
| if err != nil { | |
| errs = append(errs, err) | |
| } |
Same here
| if retries, ok := os.LookupEnv(hcloudLoadBalancersHealthCheckRetries); ok { | ||
| v, parseErr := strconv.Atoi(retries) | ||
| if parseErr != nil { | ||
| errs = append(errs, fmt.Errorf("failed to parse %s: %w", hcloudLoadBalancersHealthCheckRetries, parseErr)) | ||
| } else { | ||
| cfg.LoadBalancer.HealthCheckRetries = v | ||
| } | ||
| } |
There was a problem hiding this comment.
| if retries, ok := os.LookupEnv(hcloudLoadBalancersHealthCheckRetries); ok { | |
| v, parseErr := strconv.Atoi(retries) | |
| if parseErr != nil { | |
| errs = append(errs, fmt.Errorf("failed to parse %s: %w", hcloudLoadBalancersHealthCheckRetries, parseErr)) | |
| } else { | |
| cfg.LoadBalancer.HealthCheckRetries = v | |
| } | |
| } | |
| cfg.LoadBalancer.HealthCheckRetries, err = strconv.Atoi(os.Getenv(hcloudLoadBalancersHealthCheckRetries)) | |
| if err != nil { | |
| errs = append(errs, err) | |
| } |
Same here
| if lbType, ok := os.LookupEnv(hcloudLoadBalancersType); ok { | ||
| cfg.LoadBalancer.Type = lbType | ||
| } |
There was a problem hiding this comment.
| if lbType, ok := os.LookupEnv(hcloudLoadBalancersType); ok { | |
| cfg.LoadBalancer.Type = lbType | |
| } | |
| cfg.LoadBalancer.Type = os.Getenv(hcloudLoadBalancersType) |
cfg.LoadBalancer.Type is of type string and therefore will be initialized as an empty string. os.Getenv will return an empty string when the env is not set.
| // Workaround to keep bug https://github.com/hetznercloud/hcloud-cloud-controller-manager/issues/876 | ||
| if b.cfg.ProxyProtocolEnabled != nil { | ||
| b.proxyProtocol = hcloud.Ptr(*b.cfg.ProxyProtocolEnabled) | ||
| } |
There was a problem hiding this comment.
| // Workaround to keep bug https://github.com/hetznercloud/hcloud-cloud-controller-manager/issues/876 | |
| if b.cfg.ProxyProtocolEnabled != nil { | |
| b.proxyProtocol = hcloud.Ptr(*b.cfg.ProxyProtocolEnabled) | |
| } | |
| b.proxyProtocol = b.cfg.ProxyProtocolEnabled |
The comment is not entirely true here. The bug is about resetting the value of b.proxyProtocol when neither the annotation nor a global default is set. Essentially setting it back to its API default when the annotation is removed. Also, the comment does not describe the problem properly. The issue is, that this would be a breaking change, as users might have removed the annotation but still rely on the proxy protocol being enabled. This would silently change that.
We can also simply the statement a bit. hcloud.Ptr is just returning a pointer to the argument passed in (e.g., hcloud.Ptr(true) is a common use-case). So dereferencing the pointer and creating another reference does not add any value. Furthermore, b.proxyProtocol is initialized with nil, so if we reach this branch of the if statement we might as well just use b.cfg.ProxyProtocolEnabled directly.
| b.healthCheckOpts.Interval = hcloud.Ptr(hcInterval) | ||
| b.addHealthCheck = true | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| } else if errors.Is(err, annotation.ErrNotSet) { | ||
| if b.cfg.HealthCheckInterval != 0 { | ||
| b.healthCheckOpts.Interval = hcloud.Ptr(b.cfg.HealthCheckInterval) |
There was a problem hiding this comment.
| b.healthCheckOpts.Interval = hcloud.Ptr(hcInterval) | |
| b.addHealthCheck = true | |
| return nil | |
| } | |
| if err != nil { | |
| } else if errors.Is(err, annotation.ErrNotSet) { | |
| if b.cfg.HealthCheckInterval != 0 { | |
| b.healthCheckOpts.Interval = hcloud.Ptr(b.cfg.HealthCheckInterval) | |
| b.healthCheckOpts.Interval = &hcInterval | |
| b.addHealthCheck = true | |
| return nil | |
| } else if errors.Is(err, annotation.ErrNotSet) { | |
| if b.cfg.HealthCheckInterval != 0 { | |
| b.healthCheckOpts.Interval = &b.cfg.HealthCheckInterval |
No need to use hcloud.Ptr here. I don't know why it was used before though.
| b.healthCheckOpts.Timeout = hcloud.Ptr(t) | ||
| b.addHealthCheck = true | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| } else if errors.Is(err, annotation.ErrNotSet) { | ||
| if b.cfg.HealthCheckTimeout != 0 { | ||
| b.healthCheckOpts.Timeout = hcloud.Ptr(b.cfg.HealthCheckTimeout) |
There was a problem hiding this comment.
| b.healthCheckOpts.Timeout = hcloud.Ptr(t) | |
| b.addHealthCheck = true | |
| return nil | |
| } | |
| if err != nil { | |
| } else if errors.Is(err, annotation.ErrNotSet) { | |
| if b.cfg.HealthCheckTimeout != 0 { | |
| b.healthCheckOpts.Timeout = hcloud.Ptr(b.cfg.HealthCheckTimeout) | |
| b.healthCheckOpts.Timeout = &t | |
| b.addHealthCheck = true | |
| return nil | |
| } else if errors.Is(err, annotation.ErrNotSet) { | |
| if b.cfg.HealthCheckTimeout != 0 { | |
| b.healthCheckOpts.Timeout = &b.cfg.HealthCheckTimeout |
Similar we don't need to use hcloud.Ptr here.
| b.healthCheckOpts.Retries = hcloud.Ptr(v) | ||
| b.addHealthCheck = true | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| } else if errors.Is(err, annotation.ErrNotSet) { | ||
| if b.cfg.HealthCheckRetries != 0 { | ||
| b.healthCheckOpts.Retries = hcloud.Ptr(b.cfg.HealthCheckRetries) |
There was a problem hiding this comment.
| b.healthCheckOpts.Retries = hcloud.Ptr(v) | |
| b.addHealthCheck = true | |
| return nil | |
| } | |
| if err != nil { | |
| } else if errors.Is(err, annotation.ErrNotSet) { | |
| if b.cfg.HealthCheckRetries != 0 { | |
| b.healthCheckOpts.Retries = hcloud.Ptr(b.cfg.HealthCheckRetries) | |
| b.healthCheckOpts.Retries = &v | |
| b.addHealthCheck = true | |
| return nil | |
| } else if errors.Is(err, annotation.ErrNotSet) { | |
| if b.cfg.HealthCheckRetries != 0 { | |
| b.healthCheckOpts.Retries = &b.cfg.HealthCheckRetries |
Similar we don't need to use hcloud.Ptr here.
3 participants
This PR introduces additional environment variables for load balancer configuration. These variables are designed to be set globally as defaults and can be overridden using annotations.
The main motivation is to improve support for GatewayAPI, as the
Gatewayannotation limit of 8 is restrictive and many settings are commonly needed across all load balancers from the same or even differen GatewayAPI providers. Additionally, this change allows environment-specific presets such as the new subnet IP range to be set globally. This removes the need to configure these settings in each service or use templating/patching to use the same service manifest for different environments.New environment vars:
HCLOUD_LOAD_BALANCERS_ALGORITHM_TYPEHCLOUD_LOAD_BALANCERS_DISABLE_PUBLIC_NETWORKHCLOUD_LOAD_BALANCERS_HEALTH_CHECK_INTERVALHCLOUD_LOAD_BALANCERS_HEALTH_CHECK_RETRIESHCLOUD_LOAD_BALANCERS_HEALTH_CHECK_TIMEOUTHCLOUD_LOAD_BALANCERS_PRIVATE_SUBNET_IP_RANGEHCLOUD_LOAD_BALANCERS_TYPEHCLOUD_LOAD_BALANCERS_USES_PROXYPROTOCOL